Refactor various metric APIs to make distributed execution easier #3191
Conversation
Not having it depend on lib.TestRunState means that it can be used both by a local test run, and by a coordinator instasnce that isn't actually running the test itself.
It isn't used for anything except a few tests of questionable quality, but it's not even needed for them...
This is in preparation for HDR histograms, when the TrendSink's internals won't be the same and won't be directly accessible.
Codecov Report
@@ Coverage Diff @@
## master #3191 +/- ##
==========================================
+ Coverage 72.87% 72.89% +0.01%
==========================================
Files 256 256
Lines 19800 19806 +6
==========================================
+ Hits 14430 14437 +7
+ Misses 4470 4469 -1
Partials 900 900
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -167,10 +159,10 @@ func (me *MetricsEngine) initSubMetricsAndThresholds() error { | |||
| // StartThresholdCalculations spins up a new goroutine to crunch thresholds and | |||
| // returns a callback that will stop the goroutine and finalizes calculations. | |||
| func (me *MetricsEngine) StartThresholdCalculations( | |||
| ingester *OutputIngester, | |||
There was a problem hiding this comment.
This is already an improvement, in the final picture, I think we should try to reverse the control here. Passing the MetricsEngine as a dependency to the Ingester (the Output).
Just sharing this to you for collecting thoughts, not really a request for change.
There was a problem hiding this comment.
Yeah, this might be a better approach 🤔
Though the MetricsEngine and the OutputIngester also have other issues that I didn't fully fix when I extracted them from the core.Engine (e.g. #572 (comment) probably being the biggest one) that we can probably refactor things here even further 🤔
Still, this PR at least somewhat further decouples the metrics crunching from the test execution, so any such refactoring should hopefully be easier 🤞
What?
This PR contains parts of #2816 that are pure refactoring of various APIs, to decouple them slightly and make them more flexible and usable for the purposes of implementing distributed execution and test suites. It does not have any breaking changes and can safely be merged now. I've tried to separate the various changes into logical self-contained and small commits, so I suggest reviewing it commit by commit.
Why?
Moving these changes to a separate PR that can be merged now reduces the scope of the big PR and makes merging other backwards-compatible parts of it possible. For the parts that aren't backwards-compatible and will remain in a separate WIP PR for a long while, it makes the upkeep of the PR (i.e. rebasing it on top of
masterchanges) much, much easier.Checklist
make ci-like-lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)